-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trying to deflake CheckFilterTest
#522
Conversation
@@ -434,17 +430,4 @@ private boolean match(String s) { | |||
return Pattern.matches(pattern, s); | |||
} | |||
} | |||
|
|||
private static class SimpleBuilder extends Builder implements SimpleBuildStep, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced in #174 but never used.
@@ -439,7 +439,7 @@ private String getInfo(ContentFilter filter) { | |||
.append(" arg[") | |||
.append(count++) | |||
.append("]: `") | |||
.append(Markdown.escapeBacktick(ContentFilter.filter(filter, arg))) | |||
.append(Markdown.escapeBacktick(filter != null ? ContentFilter.filter(filter, arg) : arg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turning on fine logging revealed
FINE c.c.j.support.AsyncResultCache#get: Could not retrieve Java info from agent0
Also: hudson.remoting.Channel$CallSiteStackTrace: Remote call to agent0
at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1784)
at hudson.remoting.UserRequest$ExceptionResponse.retrieve(UserRequest.java:356)
at hudson.remoting.Channel$2.adapt(Channel.java:1034)
at hudson.remoting.Channel$2.adapt(Channel.java:1030)
at hudson.remoting.FutureAdapter.get(FutureAdapter.java:66)
at com.cloudbees.jenkins.support.AsyncResultCache.get(AsyncResultCache.java:69)
at com.cloudbees.jenkins.support.impl.AboutJenkins$NodesContent.printTo(AboutJenkins.java:808)
at com.cloudbees.jenkins.support.api.PrefilteredPrintedContent.writeTo(PrefilteredPrintedContent.java:63)
at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:420)
at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:355)
at com.cloudbees.jenkins.support.CheckFilterTest.assertComponent(CheckFilterTest.java:158)
at com.cloudbees.jenkins.support.CheckFilterTest.checkFilterTest(CheckFilterTest.java:92)
at …
java.lang.NullPointerException
at com.cloudbees.jenkins.support.filter.ContentFilter.filter(ContentFilter.java:109)
at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.getInfo(AboutJenkins.java:442)
at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.call(AboutJenkins.java:227)
at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.call(AboutJenkins.java:214)
at hudson.remoting.UserRequest.perform(UserRequest.java:211)
at …
Caused: java.util.concurrent.ExecutionException
at hudson.remoting.Channel$2.adapt(Channel.java:1036)
at hudson.remoting.Channel$2.adapt(Channel.java:1030)
at hudson.remoting.FutureAdapter.get(FutureAdapter.java:66)
at com.cloudbees.jenkins.support.AsyncResultCache.get(AsyncResultCache.java:69)
at com.cloudbees.jenkins.support.impl.AboutJenkins$NodesContent.printTo(AboutJenkins.java:808)
at com.cloudbees.jenkins.support.api.PrefilteredPrintedContent.writeTo(PrefilteredPrintedContent.java:63)
at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:420)
at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:355)
at com.cloudbees.jenkins.support.CheckFilterTest.assertComponent(CheckFilterTest.java:158)
at com.cloudbees.jenkins.support.CheckFilterTest.checkFilterTest(CheckFilterTest.java:92)
at …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this somehow is a production bug?
The filter should be filtering (anonymizing) sensitive info - and this if this is run on a JNLP agent the args may (IIUC) contain the Jenkins URL (which should be anonymized). But as stated in the javadoc this will never occur as the filter will be null for this case...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
support-core-plugin/src/main/java/com/cloudbees/jenkins/support/impl/AboutJenkins.java
Lines 816 to 820 in 14c7b90
// We make sure the output is filtered, maybe some labels are going to be filtered, but | |
// to avoid that: | |
// TODO: we have to change the MasterToSlaveCallable (GetJavaInfo) call to return a Map of | |
// values (key: value) and filter all the values here. | |
out.print(ContentFilter.filter(filter, javaInfo)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might have always been broken... See #473 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so for PII / security reasons we should (if the filter is null) not include the args at all (at least as a temporary fix to allow the deflaking of this until the code can be propertly fixed in a different ticket? (that would IIUC not loose any info from agents as that info would not be available today as the code would have have blown up?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for PII / security reasons…
Again as in #522 (comment) I think this content is already filtered, just in a clumsy way.
Anyway I file this patch in a separate PR for discussion since we need to get this test flake fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I mistakenly thought this was a partial cause of the flake (if an agent was connected at the time the bundle ran)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, probably unrelated (the flake was about the master node, weirdly enough), just something I saw when I turned on FINE
logging.
https://github.com/jenkinsci/support-core-plugin/runs/22561627825 is not reproducibe locally but has also been observed in PCT (@cloudbees reference BEE-10081). The failure does not make much sense because
AsyncResultCache.get
should always succeed on the built-in node (Jenkins
). Locally: